Media: Skip cross-origin isolation for third-party page builders#11170
Media: Skip cross-origin isolation for third-party page builders#11170adamsilverstein wants to merge 21 commits intoWordPress:trunkfrom
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @miriamelementor. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
…ation. Remove tests for the page builder action check that was moved to a separate PR (WordPress#11170). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add a check to skip Document-Isolation-Policy when a third-party page builder overrides the block editor via a custom action parameter. DIP isolates the document into its own agent cluster, which blocks same-origin iframe access that these editors rely on. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
68f7673 to
3f563a1
Compare
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Thanks for the PR @adamsilverstein
Left some feedback and questions.
| // DIP isolates the document into its own agent cluster, | ||
| // which blocks same-origin iframe access that these editors rely on. | ||
| // phpcs:ignore WordPress.Security.NonceVerification.Recommended | ||
| if ( isset( $_GET['action'] ) && 'edit' !== $_GET['action'] ) { |
There was a problem hiding this comment.
While implementing wp_set_up_cross_origin_isolation, was this issue discussed anywhere? If so, could you please share the reference links?
Discuss edge cases where
action=editis used but the block editor is replaced (e.g., Web Stories).
Also, do we have any research or data on how many plugins might be impacted by this?
There was a problem hiding this comment.
This is not an issue for Web Stories because it short-circuits here:
wordpress-develop/src/wp-includes/media.php
Lines 6453 to 6455 in 055c638
This is because it is filtering replace_editor in the same way that the latest Elementor is doing in elementor/elementor#34900.
There was a problem hiding this comment.
BTW, it also short-circuits here with the Classic Editor plugin.
There was a problem hiding this comment.
While implementing wp_set_up_cross_origin_isolation, was this issue discussed anywhere? If so, could you please share the reference links?
the original pr included this fix, but it was removed because it was deemed unrelated (#11098 (comment)).
Both changes came out of bugs reports after the release of beta 1 - https://core.trac.wordpress.org/ticket/64740 there is more discussion there about why the fix for Elementor was added
|
In my testing with Elementor without this PR things seemed to be fixed, perhaps because we switched to using Document Isolation Policy. I will confirm this with their team, then I think we can close this PR as "working as expected". |
@adamsilverstein Can you test by removing this filter? https://github.com/elementor/elementor/pull/34900/changes This fix was added on our side in 3.35 (latest version). But all our users that have < 3.35 would be affected. |
I was actually testing on an older version before upgrading and it seemed to work fine, but I will also try disabling the filter now that I have upgraded Elementor. |
|
@louiswol94 I don't get any error with that line commented out. However, I'm seeing something else unexpected here: I don't see that add_action( 'load-post.php', fn() => wp_set_up_cross_origin_isolation() );Then it does run and I get an error in the console:
And I see the Elementor loading spinner indefinitely: Why wouldn't |
|
@louiswol94 - I did verify that Elementor loads without error even with that filter removed, so I'm not sure this PR is needed. It would still be good to address Weston's question above. |
|
I can confirm that the patch fixes the issue in Elementor. However, I can only reproduce the issue if I do this change as well: --- a/src/wp-includes/default-filters.php
+++ b/src/wp-includes/default-filters.php
@@ -679,7 +679,7 @@ add_filter( 'plupload_default_settings', 'wp_show_heic_upload_error' );
// Client-side media processing.
add_action( 'admin_init', 'wp_set_client_side_media_processing_flag' );
// Cross-origin isolation for client-side media processing.
-add_action( 'load-post.php', 'wp_set_up_cross_origin_isolation' );
+add_action( 'load-post.php', fn() => wp_set_up_cross_origin_isolation() );
add_action( 'load-post-new.php', 'wp_set_up_cross_origin_isolation' );
add_action( 'load-site-editor.php', 'wp_set_up_cross_origin_isolation' );
add_action( 'load-widgets.php', 'wp_set_up_cross_origin_isolation' );(Aside from also commenting out the change introduced in elementor/elementor#34900.) |
|
Hey guys... Quick question: Does DIP break same-origin According to this https://developer.chrome.com/blog/document-isolation-policy
I get this error on my console (on http://elementorlocal.local/):
Which makes me think that if I was on a trusted origin, DIP would be enforced and I might get a security error. (This also aligns with the code comment in this PR). Then also, is it possible you cannot reproduce due to the Gutenburg plugin doing: Thanks for bearing with me here guys :D I am just trying my best to understand the full picture and make sure we reach the right conclusion. |
|
Something else that might be important evidence... I ran our test suite against this PR: https://github.com/elementor/elementor/pull/35068/changes In this PR I removed the filter and made sure the WP nightly version is used. From the logs I can see PlayWright used Chromium It ran on And all the tests passed. Meaning everything should be okay :) |
|
I will monitor this thread, but after my latest findings if you guys still do not think this PR is necessary, then I am in agreement not to merge it. I really want to thank you guys for the work you are doing! Kind regards |
@louiswol94 thanks for the continued testing here. One way to test Beta 3 on an actual site is to install the Beta Tester Plugin - https://wordpress.org/plugins/wordpress-beta-tester/. Install it and set it to download nightlies, then update. Installing the Gutenberg plugin is a good proxy, but doesn't represent the exact code WordPress will release with. If you have some sites running older Elementor versions you could test on that would provide empirical validation.
@miriamelementor - don't worry, we still have time to get this very small change into the release, but I want to make sure its really needed. We made an architectural change recently (switching the header approach) that may have already fixed the issue. The fix in this PR turns off the feature for Elementor, meaning you won't be able to use it or any of the new capabilities it provides, so i want to make sure we are turning it off for a reason! |
|
Thanks @adamsilverstein - I forgot about the Beta Tester Plugin. I am glad you prompted me to test further. I installed:
On a Live website. And get the following:
|
Right you are! I deactivated Gutenberg and then I was able to get the error, and this PR fixes the issue. |
westonruter
left a comment
There was a problem hiding this comment.
This fixes the issue with Elementor for me.
Add unit tests verifying that client-side media processing is disabled on non-secure, non-localhost origins and enabled on localhost regardless of SSL.
The NonceVerification.Recommended suppression is not needed here since $_GET['action'] is only used for a simple string comparison, not for any state-changing operation.
|
I added a couple of tests for this in 85d6e62 and it is ready for another review @westonruter @mukeshpanchal27. I'd like to get this committed asap. |
Co-authored-by: Weston Ruter <westonruter@gmail.com>
Co-authored-by: Weston Ruter <westonruter@gmail.com>
Co-authored-by: Weston Ruter <westonruter@gmail.com>
Co-authored-by: Weston Ruter <westonruter@gmail.com>
Co-authored-by: Weston Ruter <westonruter@gmail.com>
|
Checking test failures... |
The sideload route is only registered when client-side media processing is enabled, which now requires SSL or localhost. Force-enable the filter in affected tests so routes register correctly in CI.
Ah, since the CI runs on a non-SSL, non-localhost environment, wp_is_client_side_media_processing_enabled() returns false which is breaking some tests. Fixing. |
The parent set_up() creates and initializes the REST server before individual test methods run, so the filter alone is not enough. Reset the server global and re-fire rest_api_init after enabling the filter.






Summary
actionquery parameteraction=editbut replaces the block editor entirely)See discussion: #11098 (comment)
Test plan
actionparameter (e.g.action=elementor) does NOT get DIP headersaction=editis used but the block editor is replaced (e.g. Web Stories)Trac ticket: https://core.trac.wordpress.org/ticket/64803
Draft Commit Message